fix(tours): honour stripped legacy tour setting keys#1271
Conversation
📝 WalkthroughWalkthroughThe PR hardens tour completion persistence by adding legacy detection that recognizes tour-finished flags stored under hyphenated, underscored, or sanitized key variants in user-settings, then backfills them to the authoritative user meta store with a regression test validating the behavior. ChangesTour Completion Legacy Compatibility
Possibly Related PRs
Poem
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
|
Performance Test Results Performance test results for 1c3a49b are in 🛎️! Note: the numbers in parentheses show the difference to the previous (baseline) test run. Differences below 2% or 0.5 in absolute values are not shown. URL:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@inc/ui/class-tours.php`:
- Around line 130-136: The loop in is_legacy_tour_finished($id, $user_id) calls
get_user_setting($setting_key) which reads the current browser user’s settings;
change the check to read settings for the passed $user_id instead (e.g. use
get_user_option($setting_key, $user_id) or get_user_meta($user_id, $setting_key,
true)) so legacy backfill uses the intended $user_id; update the loop that
iterates get_legacy_setting_keys($id) and also ensure subsequent retrieval of
$stored_settings continues to use get_user_option('user-settings', $user_id).
In `@tests/WP_Ultimo/UI/Tours_Test.php`:
- Around line 350-356: The test must clear cookie/global user-setting state so
is_legacy_tour_finished() actually parses stripped keys: before calling
$get_legacy_keys->invoke(...) and $is_finished->invoke($instance,
'checkout-form-list', $user_id) clear any wp-settings cookie and updated-user
cache (unset $_COOKIE['wp-settings-'.$user_id] and unset
$GLOBALS['_updated_user_settings'][$user_id]) so get_user_setting() won't return
stale values; update the
test_is_tour_finished_reads_stripped_legacy_user_settings_meta() sequence to
unset those entries immediately after update_user_option(...) and before
invoking the reflection helpers ($get_legacy_keys and $is_finished).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af0148c4-f9b0-4832-b5ac-db3e92d52047
📒 Files selected for processing (2)
inc/ui/class-tours.phptests/WP_Ultimo/UI/Tours_Test.php
| foreach ($this->get_legacy_setting_keys($id) as $setting_key) { | ||
| if (get_user_setting($setting_key, false)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| $stored_settings = get_user_option('user-settings', $user_id); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In WordPress core, does get_user_setting( $name, $default ) read only the current user's wp-settings cookie/cache, or can it resolve settings for an arbitrary user ID? Please include the function signature and a short summary of its lookup behavior.
💡 Result:
Function signature: get_user_setting( $name, $default_value = false) [1] Short summary of lookup behavior (WordPress core): - get_user_setting calls get_all_user_settings, then returns the setting value from the returned array for key $name, or returns $default_value if the key is not present. [1][2] - get_all_user_settings only resolves settings for the current request’s “current user” (it uses get_current_user_id). If there is no current user ID, it returns an empty array. [2] - When a cookie is available, get_all_user_settings reads the user-settings cookie named wp-settings-{user_id} from $_COOKIE (and if not present, it falls back to the user option user-settings in the database for that same $user_id). [2] Answer to your question: In WordPress core, get_user_setting($name, $default_value) does not let you resolve settings for an arbitrary user ID. It looks up settings only for the current user ID for the current request, and it prefers the current browser’s wp-settings-{user_id} cookie (with a database fallback if the cookie is absent) rather than an arbitrary user ID argument. [2][1]
Citations:
- 1: https://developer.wordpress.org/reference/functions/get_user_setting/
- 2: https://v8-9-4.vueuse.org/integrations/usecookies/
Scope get_user_setting() to the requested user to prevent cross-user legacy backfill
get_user_setting( $name, $default_value = false ) is backed by get_all_user_settings(), which resolves settings for the current request’s current user (cookie wp-settings-{current_user_id} with DB fallback). Since is_legacy_tour_finished($id, $user_id) calls get_user_setting() without using $user_id, it can read the current browser user’s legacy cookie while then backfilling meta for the passed $user_id.
Proposed fix
protected function is_legacy_tour_finished($id, $user_id) {
- foreach ($this->get_legacy_setting_keys($id) as $setting_key) {
- if (get_user_setting($setting_key, false)) {
- return true;
- }
- }
+ if (get_current_user_id() === (int) $user_id) {
+ foreach ($this->get_legacy_setting_keys($id) as $setting_key) {
+ if (get_user_setting($setting_key, false)) {
+ return true;
+ }
+ }
+ }
$stored_settings = get_user_option('user-settings', $user_id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| foreach ($this->get_legacy_setting_keys($id) as $setting_key) { | |
| if (get_user_setting($setting_key, false)) { | |
| return true; | |
| } | |
| } | |
| $stored_settings = get_user_option('user-settings', $user_id); | |
| if (get_current_user_id() === (int) $user_id) { | |
| foreach ($this->get_legacy_setting_keys($id) as $setting_key) { | |
| if (get_user_setting($setting_key, false)) { | |
| return true; | |
| } | |
| } | |
| } | |
| $stored_settings = get_user_option('user-settings', $user_id); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@inc/ui/class-tours.php` around lines 130 - 136, The loop in
is_legacy_tour_finished($id, $user_id) calls get_user_setting($setting_key)
which reads the current browser user’s settings; change the check to read
settings for the passed $user_id instead (e.g. use get_user_option($setting_key,
$user_id) or get_user_meta($user_id, $setting_key, true)) so legacy backfill
uses the intended $user_id; update the loop that iterates
get_legacy_setting_keys($id) and also ensure subsequent retrieval of
$stored_settings continues to use get_user_option('user-settings', $user_id).
| update_user_option($user_id, 'user-settings', 'wu_tour_checkoutformlist=1', false); | ||
|
|
||
| $this->assertSame('wu_tour_checkoutformlist=1', get_user_option('user-settings', $user_id)); | ||
| $this->assertContains('wu_tour_checkoutformlist', $get_legacy_keys->invoke($instance, 'checkout-form-list')); | ||
|
|
||
| $this->assertTrue($is_finished->invoke($instance, 'checkout-form-list', $user_id)); | ||
| $this->assertSame('1', get_user_meta($user_id, $meta_key, true)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 "wp-settings-|_updated_user_settings|test_is_tour_finished_falls_back_to_legacy_user_setting|test_is_tour_finished_reads_stripped_legacy_user_settings_meta" tests/WP_Ultimo/UI/Tours_Test.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 3005
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba tests/WP_Ultimo/UI/Tours_Test.php | sed -n '320,390p'Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n tests/WP_Ultimo/UI/Tours_Test.php | sed -n '320,390p'Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 3274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "function is_tour_finished" -S . --glob='*.php'Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '130,260p' inc/ui/class-tours.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 3570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "\$_COOKIE\[['\"]wp-settings-'|_updated_user_settings|wp-settings-" tests/WP_Ultimo/UI/Tours_Test.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 637
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n tests/WP_Ultimo/UI/Tours_Test.php | sed -n '260,340p'Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 3999
🏁 Script executed:
#!/bin/bash
rg -n -C3 "wp-settings-|_updated_user_settings|test_is_tour_finished_falls_back_to_legacy_user_setting|test_is_tour_finished_reads_stripped_legacy_user_settings_meta" tests/WP_Ultimo/UI/Tours_Test.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 3005
Clear wp-settings-* cookie/cache in stripped legacy meta test
test_is_tour_finished_reads_stripped_legacy_user_settings_meta() doesn’t isolate cookie-backed get_user_setting() state. Since is_legacy_tour_finished() checks get_user_setting() before parsing get_user_option('user-settings', ...), stale $_COOKIE['wp-settings-{user_id}'] / $GLOBALS['_updated_user_settings'] can make the test pass without exercising the stripped-key parsing path.
Proposed fix
+ $cookie_name = 'wp-settings-' . $user_id;
+ $prior_cookie = $_COOKIE[ $cookie_name ] ?? null; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput -- test stash, no user input.
+ $prior_updated_settings = $GLOBALS['_updated_user_settings'] ?? null;
+ unset($_COOKIE[ $cookie_name ]);
+ $GLOBALS['_updated_user_settings'] = null;
+
- update_user_option($user_id, 'user-settings', 'wu_tour_checkoutformlist=1', false);
-
- $this->assertSame('wu_tour_checkoutformlist=1', get_user_option('user-settings', $user_id));
- $this->assertContains('wu_tour_checkoutformlist', $get_legacy_keys->invoke($instance, 'checkout-form-list'));
-
- $this->assertTrue($is_finished->invoke($instance, 'checkout-form-list', $user_id));
- $this->assertSame('1', get_user_meta($user_id, $meta_key, true));
+ try {
+ update_user_option($user_id, 'user-settings', 'wu_tour_checkoutformlist=1', false);
+
+ $this->assertSame('wu_tour_checkoutformlist=1', get_user_option('user-settings', $user_id));
+ $this->assertContains('wu_tour_checkoutformlist', $get_legacy_keys->invoke($instance, 'checkout-form-list'));
+
+ $this->assertTrue($is_finished->invoke($instance, 'checkout-form-list', $user_id));
+ $this->assertSame('1', get_user_meta($user_id, $meta_key, true));
+ } finally {
+ if (null === $prior_cookie) {
+ unset($_COOKIE[ $cookie_name ]);
+ } else {
+ $_COOKIE[ $cookie_name ] = $prior_cookie;
+ }
+ $GLOBALS['_updated_user_settings'] = $prior_updated_settings;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/WP_Ultimo/UI/Tours_Test.php` around lines 350 - 356, The test must
clear cookie/global user-setting state so is_legacy_tour_finished() actually
parses stripped keys: before calling $get_legacy_keys->invoke(...) and
$is_finished->invoke($instance, 'checkout-form-list', $user_id) clear any
wp-settings cookie and updated-user cache (unset
$_COOKIE['wp-settings-'.$user_id] and unset
$GLOBALS['_updated_user_settings'][$user_id]) so get_user_setting() won't return
stale values; update the
test_is_tour_finished_reads_stripped_legacy_user_settings_meta() sequence to
unset those entries immediately after update_user_option(...) and before
invoking the reflection helpers ($get_legacy_keys and $is_finished).
SummaryFixes remaining admin tours reappearing for users whose dismissed-tour state was saved before the tour ID normalization fix.
Root causePrior fixes covered newer persistence paths:
Fix
Verification
NotesThe broader aidevops.sh v3.19.0 plugin for OpenCode v1.15.10 with gpt-5.5 spent 5h 33m and 14,327 tokens on this as a headless worker. Merged via PR #1271 to main. |
superdav42
added
the
review-feedback-scanned
…peating on every page load (#1281) * fix(tours): mark one-shot tours as finished on render to stop them repeating on every page load When a tour was rendered without the user explicitly clicking through to the last step (e.g. they refreshed the page, navigated away mid-walkthrough, closed the browser tab, or hit the back button before reaching the close button), the wu_tour_finished_* meta flag was never written. The next page load would re-render the same tour, producing the user-visible symptom of "the same admin tour keeps appearing on every page load". Previous fixes (#1051, #1268, #1271, #1277) ensured the dismissal *could* persist across cookie / id-normalization / user-scoping edge cases — but all of them still depended on the AJAX dismissal triggered by Shepherd's complete / cancel events. If the user never reached the end of the tour, no AJAX call was made and no flag was stored. Persist the finished flag synchronously, server-side, the moment a one-shot tour is queued for display. The Shepherd event handlers in tours.js still fire markTourFinished for completeness; update_user_meta is idempotent so the double-write is harmless. Tours registered with $once = false continue to render on every page load. Verified on https://ruling-sable.jurassic.ninja (Ultimate Multisite v2.12.0 deploy) by: - Resetting wu_tour_* user meta and wp_user-settings for the demo user in a brand new agent-browser session. - Loading /wp-admin/network/admin.php?page=wp-ultimo — tour renders once, wu_tour_finished_wp_ultimo_dashboard = 1 written immediately. - Reloading the same URL — tour no longer renders. - Repeating for /wp-admin/network/admin.php?page=wp-ultimo-checkout-forms (checkout-form-list) — same one-shot behaviour confirmed. * fix(tours): avoid rewriting filter-forced tour state * fix(e2e): stabilize Cypress login and password reset fixture
1 participant
Summary
Fixes remaining admin tours reappearing for users whose dismissed-tour state was saved before the tour ID normalization fix.
Affected URLs observed locally:
wp-admin/network/admin.php?page=wp-ultimo-checkout-formswp-admin/network/admin.php?page=wp-ultimoRoot cause
Prior fixes covered newer persistence paths:
wu_tour_finished_*user meta for newly dismissed tours.However, older dismissals can still exist in WordPress
user-settingswith hyphens stripped entirely by the legacy cookie/settings path, for example:wu_tour_checkoutformlist=1wu_tour_wpultimodashboard=1Current code checks the new meta key and the normalized legacy setting key, but not these stripped historical keys. Those users keep seeing tours they already dismissed.
Fix
user-settingsdirectly withget_user_option('user-settings', $user_id)whenget_user_setting()does not find the normalized key.wu_tour_finished_*meta flag so future reads use the modern source of truth.Verification
vendor/bin/phpunit --filter test_is_tour_finished_reads_stripped_legacy_user_settings_meta --no-coverage→ passed when mirrored into the dev checkout.vendor/bin/phpunit --filter 'test_is_tour_finished_(reads_user_meta|falls_back_to_legacy_user_setting|reads_stripped_legacy_user_settings_meta)' --no-coverage→OK (3 tests, 9 assertions)when mirrored into the dev checkout.vendor/bin/phpstan analyse inc/ui/class-tours.php→ no errors.wu_tour_checkoutformlist=1&wu_tour_wpultimodashboard=1window.wu_toursnull,.shepherd-elementcount 0window.wu_toursnull,.shepherd-elementcount 0wu_tour_finished_checkout_form_list=1andwu_tour_finished_wp_ultimo_dashboard=1Notes
The broader
Tours_Testfile still has a pre-existing PHPCS warning forwp_register_script(..., false, ...)in an unrelated test path; this PR does not introduce that warning.aidevops.sh v3.19.0 plugin for OpenCode v1.15.10 with gpt-5.5 spent 5h 33m and 14,327 tokens on this as a headless worker.
Summary by CodeRabbit
Bug Fixes
Tests